feat: Support for mappingproxy objects #3521
Conversation
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
Signed-off-by: Sam Ezeh <[email protected]>
michaelhly
force-pushed
the
support-mappingproxy
branch
from
14ecd2f
to
54fdc00
Compare
michaelhly
force-pushed
the
support-mappingproxy
branch
from
a07abd4
to
bbc60b9
Compare
michaelhly
changed the title
michaelhly
force-pushed
the
support-mappingproxy
branch
2 times, most recently
from
85b554f
to
709ee4a
Compare
michaelhly
force-pushed
the
support-mappingproxy
branch
from
709ee4a
to
e949a7e
Compare
michaelhly
force-pushed
the
support-mappingproxy
branch
from
de6b7d4
to
16df9bf
Compare
michaelhly
force-pushed
the
support-mappingproxy
branch
from
6081759
to
5005fc3
Compare
There was a problem hiding this comment.
Thank you for this implementation, and sorry it's taken me a few days to reply!
Overall looks very good, but I would like to see some changes to propagate errors rather than introduce panics. The only case where I'm not 100% sure is in the iterator, but there I still think it's better to err on the side of fallibility. Would be interesting to see what other maintainers think.
Also, would you be willing to help us prepare this new API in a forwards-compatible way for #3382? I have started some other PRs like #3535 and #3531 which hopefully show you what I'm doing at the moment, we could add PyMappingProxyMethods here in the same way. It would be really interesting to get your opinion as a user how this new API feels, though I confess that it's still early days and unrefined so you might be in for a bumpy ride 🙏
| pub fn is_empty(&self) -> bool { | ||
| self.len().unwrap_or_default() == 0 | ||
| } |
There was a problem hiding this comment.
I think we can't justify this default for users and should propagate the error:
| pub fn is_empty(&self) -> bool { | |
| self.len().unwrap_or_default() == 0 | |
| } | |
| pub fn is_empty(&self) -> PyResult<bool> { | |
| self.len().map(|len| len == 0) | |
| } |
| pub fn get_item<K>(&self, key: K) -> Option<&PyAny> | ||
| where | ||
| K: ToPyObject, | ||
| { | ||
| PyAny::get_item(self, key).ok() | ||
| } |
There was a problem hiding this comment.
Given the recent changes around PyDict::get_item, similarly we should not suppress the error here. As there's no clear advantage of this over PyAny::get_item I guess we just remove this?
| pub fn keys(&self) -> &PyList { | ||
| unsafe { | ||
| PySequence::try_from_unchecked(self.call_method0("keys").unwrap()) | ||
| .to_list() | ||
| .unwrap() | ||
| } | ||
| } |
There was a problem hiding this comment.
The unwraps are not ok here, let's have PyResult<&PyList> as the return type please!
| /// Returns a list of mappingproxy values. | ||
| /// | ||
| /// This is equivalent to the Python expression `list(mappingproxy.values())`. | ||
| pub fn values(&self) -> &PyList { |
There was a problem hiding this comment.
Same here PyResult<&PyList>
| /// Returns a list of mappingproxy items. | ||
| /// | ||
| /// This is equivalent to the Python expression `list(mappingproxy.items())`. | ||
| pub fn items(&self) -> &PyList { |
There was a problem hiding this comment.
Same here PyResult<&PyList>
| /// Conversion trait that allows a sequence of tuples to be converted into `PyMappingProxy` | ||
| /// Primary use case for this trait is `call` and `call_method` methods as keywords argument. | ||
| pub trait IntoPyMappingProxy<'py> { | ||
| /// Converts self into a `PyMappingProxy` object pointer. Whether pointer owned or borrowed | ||
| /// depends on implementation. | ||
| fn into_py_mappingproxy(self, py: Python<'py>) -> PyResult<&'py PyMappingProxy>; | ||
| } | ||
|
|
||
| impl<'py, T, I> IntoPyMappingProxy<'py> for I | ||
| where | ||
| T: PyDictItem, | ||
| I: IntoIterator<Item = T>, | ||
| { | ||
| fn into_py_mappingproxy(self, py: Python<'py>) -> PyResult<&'py PyMappingProxy> { | ||
| let dict = self.into_py_dict(py); | ||
| PyMappingProxy::new(py, dict.as_mapping()) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a cute trait, but I'm not really convinced this is necessary given the user can just do PyMappingProxy::new(py, v.into_py_dict(py).as_mapping()). Have you got a use case in mind?
| } | ||
|
|
||
| impl<'py> Iterator for PyMappingProxyIterator<'py> { | ||
| type Item = (&'py PyAny, &'py PyAny); |
There was a problem hiding this comment.
I guess iteration here should probably be fallible? Overall I see why you added this and I think it makes sense to iterate pairs given the PyDictIterator works the same way.
| type Item = (&'py PyAny, &'py PyAny); | |
| type Item = PyResult<(&'py PyAny, &'py PyAny)>; |
|
Continued in #4644 |
Adds support for mappingproxy objects. Continuation of #2435. Resolves #2108.